Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove max file size check in openExistingOrNew #50

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

coderbirju
Copy link

The current implementation of logrotate.openExistingOrNew has multiple checks to verify if there is an memory overflow on the existing log file and rotates it if true.

This code in particular checks the current log file size when logger.size is not yet set. (which defaults l.size to 0)
The max function always returns a number less than filesize + writeLen resulting in the rotate function being called. This behavior is seen when maxBytes is set to -1.

This results in nerdctl container logs failing to persist in the same log file as each new write rotates the logs and overwrites the older log file.
nerdctl issue

@@ -284,10 +284,6 @@ func (l *Logger) openExistingOrNew(writeLen int) error {
return fmt.Errorf("error getting log file info: %s", err)
}

if info.Size()+int64(writeLen) >= l.max(int64(writeLen)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets say we set a size and the new check starts just at the border when the file is at max size but not yet at the tipping point, removing this check can potentially make the file not rotate?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function in called within the write function where there is another check to ensure that the logs don't overflow.
if l.size+writeLen > l.max(writeLen)

@fahedouch fahedouch added this to the v0.2.2 milestone Nov 16, 2024
@fahedouch
Copy link
Owner

Thanks @coderbirju, could you please add a test to control this behavior!

@coderbirju coderbirju force-pushed the update-logrotate branch 2 times, most recently from 900978e to 6aa59a1 Compare November 19, 2024 01:04
Signed-off-by: Arjun Raja Yogidas <[email protected]>
@coderbirju
Copy link
Author

Updated the comments and added a test case for the openExistingOrNew function. PTAL. Thanks

@fahedouch
Copy link
Owner

fahedouch commented Nov 24, 2024

Updated the comments and added a test case for the openExistingOrNew function. PTAL. Thanks

Thanks. Is it possible to make test with MaxBytes equals to -1 to test the behavior that you mentionned in the description ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants